-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RTO-35842 Allow multiple ids to show function #34
RTO-35842 Allow multiple ids to show function #34
Conversation
lib/moodle_rb/courses.rb
Outdated
} | ||
} | ||
}.merge(query_options) | ||
) | ||
check_for_errors(response) | ||
|
||
return response.parsed_response if response.parsed_response.count > 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the Moodle API do if you call it with course IDs in the list that do not exist? Hopefully it will skip those and give us just the ones it has. If that's the case, it's possible we may only have 1, or even 0, in the response, even though the caller has passed multiple ids and is expecting an array back, so I think we should return an array (even of 0 or 1 item) if the user has passed more than one id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually going to suggest a change to this to a ternary statement, but then got sidetracked.
I do think we should check the number of id
s passed in as described above.
lib/moodle_rb/courses.rb
Outdated
:ids => { | ||
'0' => id | ||
} | ||
id: ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be my fault as I probably got it wrong in the suggestion, but I've noticed the parameter key name here should be ids
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Fix gem build issue
Ticket: https://jobready.atlassian.net/browse/RTO-35842
Update the moodle-rb gem to allow multiple ids to Courses show function.